Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature gate: init program with RevokePendingActivation instruction #5510

Merged
merged 3 commits into from
Oct 18, 2023
Merged

feature gate: init program with RevokePendingActivation instruction #5510

merged 3 commits into from
Oct 18, 2023

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Oct 11, 2023 •
edited
Loading

This PR is the first in a series of re-written commit history to facilitate easier discussion on the Feature Gate Program, as SIMD 0066 progresses.

Original PR: #4987

Notable changes:

  • Condensed feedback to one commit, defining the "base" program
  • Only defines the RevokePendingActivation instruction
  • Removed CLI (for now)

@buffalojoec buffalojoec requested a review from joncinque October 11, 2023 15:20
@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from a5b763a to 4d129fc Compare October 11, 2023 15:37
@buffalojoec buffalojoec mentioned this pull request Oct 11, 2023
Closed
joncinque
joncinque reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great! The revoke logic is perfect, so maybe this PR should just do that, and we can discuss some better activation behavior

feature-gate/program/src/entrypoint.rs Show resolved Hide resolved
feature-gate/program/src/instruction.rs Outdated Show resolved Hide resolved
feature-gate/program/src/instruction.rs Outdated Show resolved Hide resolved
feature-gate/program/src/instruction.rs Outdated Show resolved Hide resolved
feature-gate/program/src/error.rs Outdated Show resolved Hide resolved
feature-gate/program/tests/functional.rs Outdated Show resolved Hide resolved
@buffalojoec
Copy link
Contributor Author

buffalojoec commented Oct 13, 2023 •
edited
Loading

Really great! The revoke logic is perfect, so maybe this PR should just do that, and we can discuss some better activation behavior

Sounds good, I've re-written commit history so this PR reflects only the RevokePendingActivation instruction, and I've addressed all feedback!

@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from 4d129fc to 26b4365 Compare October 13, 2023 14:24
@buffalojoec buffalojoec changed the title feature gate: init program feature gate: init program with RevokePendingActivation instruction Oct 13, 2023
joncinque
joncinque reviewed Oct 13, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's pretty much there! Just a few last bits, then this is ready

Cargo.toml Outdated Show resolved Hide resolved
feature-gate/README.md Outdated Show resolved Hide resolved
feature-gate/src/error.rs Outdated Show resolved Hide resolved
@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from 26b4365 to 3411e13 Compare October 15, 2023 17:26
@buffalojoec Graphite App
Copy link
Contributor Author

buffalojoec commented Oct 15, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from 4144973 to 3d85831 Compare October 16, 2023 08:11
@buffalojoec buffalojoec mentioned this pull request Oct 16, 2023
Closed
@buffalojoec
Copy link
Contributor Author

It's worth noting that the test I've set up here is using solana_program::feature::activate_with_lamports, so it demonstrates that the RevokePendingActivation instruction will work out-of-the-box with the current feature activation process!

@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch 2 times, most recently from cb72e2a to e4afcb4 Compare October 17, 2023 10:25
This was referenced Oct 17, 2023
Closed
Closed
@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from e4afcb4 to 5e3ee34 Compare October 17, 2023 11:10
joncinque
joncinque previously approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but let's also get an approval from @CriesofCarrots

CriesofCarrots
CriesofCarrots reviewed Oct 17, 2023
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially just nits

feature-gate/program/src/processor.rs Outdated Show resolved Hide resolved
feature-gate/program/src/instruction.rs Outdated Show resolved Hide resolved
feature-gate/program/src/processor.rs Outdated Show resolved Hide resolved
@buffalojoec buffalojoec force-pushed the 10-11-feature_gate_init_program branch from 5e3ee34 to f202fdb Compare October 18, 2023 08:09
@mergify mergify bot dismissed joncinque’s stale review October 18, 2023 08:10

Pull request has been modified.

CriesofCarrots
CriesofCarrots approved these changes Oct 18, 2023
View reviewed changes
feature-gate/program/src/processor.rs
return Err(ProgramError::IllegalOwner);
}

// This will also check the program ID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@buffalojoec buffalojoec merged commit fe6a570 into master Oct 18, 2023
6 checks passed
@buffalojoec buffalojoec deleted the 10-11-feature_gate_init_program branch October 18, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CriesofCarrots CriesofCarrots CriesofCarrots approved these changes

@joncinque joncinque joncinque left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@buffalojoec @joncinque @CriesofCarrots